-
Notifications
You must be signed in to change notification settings - Fork 59
cores: arduino: zephyrCommon Specify port/pin instead of gpio_dt_spec
#148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
gpio_dt_spec.
gpio_dt_spec.gpio_dt_spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the Arduino-on-Zephyr core to prefer direct GPIO port/pin usage over gpio_dt_spec, and adjusts build logic to better select board-/revision-specific devicetree overlays and variant directories.
Changes:
- Refactor GPIO access in
zephyrCommon.cppto use helpers that expose GPIO port + pin for common operations (digital I/O, tone, pulseIn, interrupts). - Update sample
CMakeLists.txtfiles to prefer a${NORMALIZED_BOARD_TARGET}_${BOARD_REVISION}.overlaywhen present, falling back to${NORMALIZED_BOARD_TARGET}.overlay. - Adjust top-level variant directory selection to look for
variants/${BOARD}orvariants/${NORMALIZED_BOARD_TARGET}.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Updates variant directory discovery to support additional naming patterns. |
| cores/arduino/zephyrCommon.cpp | Refactors GPIO operations to use port/pin helpers instead of _dt convenience APIs in multiple call sites. |
| samples/analog_input/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/attach_interrupt/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/blinky_arduino/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/button_press_led/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/fade/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/hello_arduino/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/i2cdemo/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/serial_event/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/spi_controller/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
| samples/threads_arduino/CMakeLists.txt | Adds overlay selection logic with optional board revision overlay. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2c89225 to
1567002
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1567002 to
6e983d1
Compare
In preparation for improvements to allow the use of GPIOs without device tree definitions, the interface will be changed to specify port and pin instead of `gpio_dt_spec`. Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
6e983d1 to
46287d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constexpr inline const struct device *local_gpio_port(pin_size_t gpin) { | ||
| return arduino_pins[gpin].port; | ||
| } | ||
|
|
||
| constexpr inline pin_size_t local_gpio_pin(pin_size_t gpin) { | ||
| return arduino_pins[gpin].pin; | ||
| } | ||
|
|
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper functions local_gpio_port() and local_gpio_pin() only extract the port and pin from gpio_dt_spec, discarding the dt_flags field. This means GPIO_ACTIVE_LOW flags defined in devicetree (e.g., variants/nrf52840dk_nrf52840/nrf52840dk_nrf52840.overlay:25) are no longer respected by functions that use these helpers. To maintain backward compatibility and support active-low GPIOs, either continue using the *_dt API variants (gpio_pin_set_dt, gpio_pin_get_dt, gpio_pin_toggle_dt), or create additional helpers that apply dt_flags inversion when needed.
| constexpr inline const struct device *local_gpio_port(pin_size_t gpin) { | |
| return arduino_pins[gpin].port; | |
| } | |
| constexpr inline pin_size_t local_gpio_pin(pin_size_t gpin) { | |
| return arduino_pins[gpin].pin; | |
| } | |
| constexpr inline const struct gpio_dt_spec &local_gpio_dt_spec(pin_size_t gpin) { | |
| return arduino_pins[gpin]; | |
| } | |
| inline int local_gpio_pin_set(pin_size_t gpin, int value) { | |
| return gpio_pin_set_dt(&arduino_pins[gpin], value); | |
| } | |
| inline int local_gpio_pin_get(pin_size_t gpin) { | |
| return gpio_pin_get_dt(&arduino_pins[gpin]); | |
| } | |
| inline int local_gpio_pin_toggle(pin_size_t gpin) { | |
| return gpio_pin_toggle_dt(&arduino_pins[gpin]); | |
| } |
| gpio_pin_set(local_gpio_port(pt->pin), local_gpio_pin(pt->pin), 0); | ||
| } else { | ||
| gpio_pin_toggle_dt(spec); | ||
| gpio_pin_toggle(local_gpio_port(pt->pin), local_gpio_pin(pt->pin)); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_toggle_dt() to gpio_pin_toggle() loses GPIO_ACTIVE_LOW flag handling. For active-low pins, the toggle behavior may be inverted. Either use gpio_pin_toggle_dt() or manually account for dt_flags.
| gpio_pin_toggle(local_gpio_port(pt->pin), local_gpio_pin(pt->pin)); | |
| gpio_pin_toggle_dt(&arduino_pins[pt->pin]); |
| k_timer_init(timer, tone_expiry_cb, NULL); | ||
|
|
||
| gpio_pin_set_dt(spec, 0); | ||
| gpio_pin_set(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), 0); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_set_dt() to gpio_pin_set() loses GPIO_ACTIVE_LOW flag handling. For active-low pins, the timer start reset behavior will be incorrect. Either use gpio_pin_set_dt() or manually account for dt_flags inversion.
|
|
||
| k_timer_stop(&arduino_pin_timers[pinNumber].timer); | ||
| gpio_pin_set_dt(spec, 0); | ||
| gpio_pin_set(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), 0); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_set_dt() to gpio_pin_set() loses GPIO_ACTIVE_LOW flag handling. For active-low pins, the noTone reset behavior will be incorrect. Either use gpio_pin_set_dt() or manually account for dt_flags inversion.
| while(gpio_pin_get(port, pin) == state && k_timer_status_get(&timer) == 0); | ||
| if (k_timer_status_get(&timer) > 0) { | ||
| goto cleanup; | ||
| } | ||
|
|
||
| while(gpio_pin_get_dt(spec) != state && k_timer_status_get(&timer) == 0); | ||
| while(gpio_pin_get(port, pin) != state && k_timer_status_get(&timer) == 0); | ||
| if (k_timer_status_get(&timer) > 0) { | ||
| goto cleanup; | ||
| } | ||
|
|
||
| start = k_uptime_ticks(); | ||
| while(gpio_pin_get_dt(spec) == state && k_timer_status_get(&timer) == 0); | ||
| while(gpio_pin_get(port, pin) == state && k_timer_status_get(&timer) == 0); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from device_is_ready(spec->port) via gpio_is_ready_dt() to device_is_ready(port) is correct for checking port readiness. However, the subsequent gpio_pin_get() calls on lines 462, 467, and 473 lose GPIO_ACTIVE_LOW flag handling. For active-low pins, the state comparisons will be inverted. Either use gpio_pin_get_dt() or manually invert based on dt_flags.
|
|
||
| void digitalWrite(pin_size_t pinNumber, PinStatus status) { | ||
| gpio_pin_set_dt(&arduino_pins[pinNumber], status); | ||
| gpio_pin_set(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), status); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_set_dt() to gpio_pin_set() loses the GPIO_ACTIVE_LOW flag handling. For pins configured with GPIO_ACTIVE_LOW in devicetree (present in nrf52840dk_nrf52840.overlay and nrf9160dk_nrf9160.overlay), digitalWrite(pin, HIGH) will now drive the physical line high instead of low, inverting the intended behavior. Either use gpio_pin_set_dt() to preserve dt_flags handling, or manually invert the status parameter based on arduino_pins[pinNumber].dt_flags.
| gpio_pin_set(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), status); | |
| gpio_pin_set_dt(&arduino_pins[pinNumber], status); |
| if (pt->count == 0) { | ||
| k_timer_stop(timer); | ||
| gpio_pin_set_dt(spec, 0); | ||
| gpio_pin_set(local_gpio_port(pt->pin), local_gpio_pin(pt->pin), 0); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_set_dt() to gpio_pin_set() in the tone stop path loses GPIO_ACTIVE_LOW flag handling. For active-low pins, setting value 0 may actually assert the pin instead of de-asserting it. Either use gpio_pin_set_dt() or manually account for dt_flags inversion.
|
|
||
| if (frequency == 0) { | ||
| gpio_pin_set_dt(spec, 0); | ||
| gpio_pin_set(local_gpio_port(pinNumber), local_gpio_pin(pinNumber), 0); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching from gpio_pin_set_dt() to gpio_pin_set() loses GPIO_ACTIVE_LOW flag handling. For active-low pins, the initial reset behavior will be incorrect. Either use gpio_pin_set_dt() or manually account for dt_flags inversion.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In preparation for improvements to allow the use of GPIOs without
device tree definitions,
the interface will be changed to specify port and pin instead of
gpio_dt_spec.